Fixed `EADDRINUSE` issue for tests running in parallel #3068

sergei-startsev · 2018-06-29T15:56:32Z

Fixed #3065 by passing handle that has already been bound to a port, see server.listen.

googlebot · 2018-06-29T15:56:35Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

googlebot · 2018-06-29T16:34:36Z

CLAs look good, thanks!

lusarz

Good job, I did a quick test, and it seems to works as expected now.

Just a few small things to consider, please either fix or add comment, then I'll approve.

lusarz · 2018-07-01T16:12:00Z

lib/utils/net-utils.js

-    return new Promise((resolve, reject) => {
-      const server = net.createServer()
+  getAvailablePort (port, listenAddress) {
+    function listen (port, listenAddress) {


Why we need to create listen function, and invoke it right after ? Can we return new Promise directly in getAvailablePort method ?

lusarz · 2018-07-01T16:19:25Z

lib/server.js

@@ -117,8 +117,11 @@ class Server extends KarmaEventEmitter {
      BundleUtils.bundleResourceIfNotExist('context/main.js', 'static/context.js')
    ])
      .then(() => NetUtils.getAvailablePort(config.port, config.listenAddress))
-      .then((port) => {
+      .then((bound) => {


Please consider using array decomposition: then(([port, server]) => { ... } ).

I believe Node 4 doesn't support destructuring: https://node.green/#ES2015-syntax-destructuring--parameters

lusarz · 2018-07-01T16:23:43Z

lib/utils/net-utils.js

-  isPortAvailable (port, listenAddress) {
-    return new Promise((resolve, reject) => {
-      const server = net.createServer()
+  getAvailablePort (port, listenAddress) {


What do you think about changing name of this function ? Now it works a bit different, so it would be nice to change name of function to be more descriptive. Maybe one of boundPort, takePort, findAndBoundAvailablePort - what do you think ?

bind is the correct verb - findAndBindAvailablePort()

asapach · 2018-07-01T16:49:31Z

lib/utils/net-utils.js

-      server.listen(port, listenAddress, () => {
-        server.close(() => resolve(true))
+        server.listen(port, listenAddress, () => {
+          resolve([port, server])


I believe you can get the port as server.address().port: https://nodejs.org/api/net.html#net_server_address
So maybe no need to pass it as a param.

@sergei-startsev What do you think about that ?

asapach · 2018-07-01T16:51:04Z

lib/utils/net-utils.js

+        server.on('error', (err) => {
+          server.close()
+          if (err.code === 'EADDRINUSE' || err.code === 'EACCES') {
+            server.listen(++port, listenAddress)


You're not passing the callback here - does the one below get invoked?

sergei-startsev · 2018-07-01T22:02:09Z

Folks, I've updated PR, could you take a look one more time?

lusarz · 2018-07-02T06:45:36Z

lib/utils/net-utils.js

+            reject(err)
+          }
+        })
+        .on('listening', () => {


Really nice 👍

Just to be sure - did you tested if #3011 don't occurs ?

Since there're no clear repro steps, I've tried to test it on a project with thousands tests running in parallel - I haven't seen any issues.

sergei-startsev · 2018-07-03T19:57:43Z

lib/utils/net-utils.js

+          }
+        })
+        .on('listening', () => {
+          resolve(server)


API has been changed a bit - now it returns only a bound server

asapach · 2018-07-03T20:02:52Z

test/unit/server.spec.js

+    sinon
+      .stub(server._injector, 'get')
+      .withArgs('webServer')
+      .returns(mockWebServer)


minor: I would put returns() on the same line as the corresponding withArgs()

asapach · 2018-07-03T20:04:34Z

test/unit/server.spec.js

+      sinon
+        .stub(server, 'get')
+        .withArgs('config')
+        .returns(config)


…ner#3065)

sergei-startsev · 2018-07-11T10:17:57Z

Is there a chance to merge the PR?

lusarz · 2018-07-11T10:24:31Z

Yes, I approved it. You should be able to merge it yourself.

sergei-startsev · 2018-07-11T12:41:40Z

@lusarz I see Only those with write access to this repository can merge pull requests.

lusarz · 2018-07-11T12:44:50Z

@sergei-startsev Ok, I thought that it works in another way - merged :)

sergei-startsev · 2018-07-11T12:48:46Z

thanks!

sergei-startsev force-pushed the addr-in-use-fix branch from b12a496 to 6cb6d12 Compare June 29, 2018 16:34

johnjbarton requested a review from lusarz June 29, 2018 16:36

sergei-startsev force-pushed the addr-in-use-fix branch from 6cb6d12 to 1777152 Compare June 29, 2018 16:48

lusarz reviewed Jul 1, 2018

View reviewed changes

asapach reviewed Jul 1, 2018

View reviewed changes

sergei-startsev force-pushed the addr-in-use-fix branch 2 times, most recently from dc25020 to 0f78818 Compare July 1, 2018 21:07

lusarz reviewed Jul 2, 2018

View reviewed changes

sergei-startsev force-pushed the addr-in-use-fix branch from 0f78818 to acdb61c Compare July 3, 2018 19:45

sergei-startsev commented Jul 3, 2018

View reviewed changes

asapach reviewed Jul 3, 2018

View reviewed changes

test/unit/server.spec.js Outdated

sinon

.stub(server, 'get')

.withArgs('config')

.returns(config)

Copy link

asapach Jul 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

fix(server): pass bound port to preventEADDRINUSE issue. (karma-run…

2b3a32c

…ner#3065)

sergei-startsev force-pushed the addr-in-use-fix branch from acdb61c to 2b3a32c Compare July 3, 2018 20:25

lusarz approved these changes Jul 4, 2018

View reviewed changes

lusarz merged commit 850a90b into karma-runner:master Jul 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed `EADDRINUSE` issue for tests running in parallel #3068

Fixed `EADDRINUSE` issue for tests running in parallel #3068

sergei-startsev commented Jun 29, 2018

googlebot commented Jun 29, 2018

googlebot commented Jun 29, 2018

lusarz left a comment

lusarz Jul 1, 2018

lusarz Jul 1, 2018

asapach Jul 1, 2018

lusarz Jul 1, 2018

lusarz Jul 1, 2018

asapach Jul 1, 2018

asapach Jul 1, 2018

lusarz Jul 2, 2018

asapach Jul 1, 2018

sergei-startsev commented Jul 1, 2018

lusarz Jul 2, 2018

lusarz Jul 2, 2018

sergei-startsev Jul 3, 2018 •

edited

Loading

sergei-startsev Jul 3, 2018

asapach Jul 3, 2018

sergei-startsev Jul 3, 2018

asapach Jul 3, 2018

sergei-startsev commented Jul 11, 2018

lusarz commented Jul 11, 2018

sergei-startsev commented Jul 11, 2018

lusarz commented Jul 11, 2018

sergei-startsev commented Jul 11, 2018

Fixed EADDRINUSE issue for tests running in parallel #3068

Fixed EADDRINUSE issue for tests running in parallel #3068

Conversation

sergei-startsev commented Jun 29, 2018

googlebot commented Jun 29, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Jun 29, 2018

lusarz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergei-startsev commented Jul 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergei-startsev Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergei-startsev commented Jul 11, 2018

lusarz commented Jul 11, 2018

sergei-startsev commented Jul 11, 2018

lusarz commented Jul 11, 2018

sergei-startsev commented Jul 11, 2018

Fixed `EADDRINUSE` issue for tests running in parallel #3068

Fixed `EADDRINUSE` issue for tests running in parallel #3068

sergei-startsev Jul 3, 2018 •

edited

Loading